[processor] Save the merged report and infer labels from browser_channel #529
Conversation
| @@ -168,11 +168,19 @@ def task_handler(): | |||
| # to tell TaskQueue to drop the task. | |||
| return ('', HTTPStatus.NO_CONTENT) | |||
|
|
|||
| resp = "{} results loaded from {}\n".format(len(report.results), gcs_path) | |||
| resp = "{} results loaded from {}\n".format( | |||
| len(report.results), str(gcs_paths)) | |||
There was a problem hiding this comment.
For other reviewers, what changed here is str(gcs_paths).
There was a problem hiding this comment.
FWIW, gcs_path is the last item in gcs_paths, the iteration variable, and it wasn't intended to be used outside of the loop.
results-processor/wptreport.py
Outdated
| def serialize_gzip(self, filepath): | ||
| """Serializes and gzips the in-memory report to a file. | ||
|
|
||
| finalize() should be called first. |
There was a problem hiding this comment.
What happens otherwise? Can anything be asserted to make sure it has been called?
There was a problem hiding this comment.
In fact, on a second thought, there's no need to call finalize here, as this method only serializes the report itself, not the metadata or summary. Let me remove this line.
results-processor/wptreport.py
Outdated
| labels.add('stable') | ||
| # Extract browser_channel into a label; default to "stable". | ||
| if report.run_info.get('browser_channel') in {'nightly', 'dev'}: | ||
| labels.add('experimental') |
There was a problem hiding this comment.
Can you split out this change? Letting the experimental label be implied seems risky if we're going to let it mean "dev with extra features enabled".
There was a problem hiding this comment.
Let me split it into a separate commit, but still in this PR.
There was a problem hiding this comment.
Thanks! I guess you usually merge for PRs where you've made the history nice?
WDYT about "Letting the experimental label be implied seems risky"?
There was a problem hiding this comment.
FWIW, there is another issue here, meaning that in https://staging.wpt.fyi/test-runs?label=taskcluster two of the runs for 9614def are identified as stable.
There was a problem hiding this comment.
Ahh those are beta results. Let me add the "beta" label here.
There was a problem hiding this comment.
@foolip I just updated the second commit in this PR to add the "beta" label.
There was a problem hiding this comment.
Cool! Is there an issue for adding beta icons as well?
This change makes the results processor to save the merged report instead of just the last chunk of the report if the original report is chunked when uploaded.
Hexcles
force-pushed
the
save-merged-report
branch
from
96482c7
to
f9981f9
Compare
|
@foolip PTAL again. |
Hexcles
changed the title
|
Staging instance deployed by Travis CI! |
results-processor/wptreport.py
Outdated
| labels.add('stable') | ||
| # Extract browser_channel into a label; default to "stable". | ||
| if report.run_info.get('browser_channel') in {'nightly', 'dev'}: | ||
| labels.add('experimental') |
There was a problem hiding this comment.
Thanks! I guess you usually merge for PRs where you've made the history nice?
WDYT about "Letting the experimental label be implied seems risky"?
|
@foolip ah you're actually the only reviewer for this PR :) I sent the Go part to Luke (#536 ). Regarding the risks of inferring labels, I think it's pretty low: we only infer it when the uploader doesn't set stable or experimental, and when |
When the uploader does not provide a channel label, the processor will now set it based on a new property in the report, `run_info.browser_channel`, introduced in web-platform-tests/wpt#12679 . A new channel label, "beta", is also introduced in this commit, since we now have beta browsers running on Taskcluster daily.
Hexcles
force-pushed
the
save-merged-report
branch
from
f9981f9
to
9521c10
Compare
This change makes the results processor to save the merged report
instead of just the last chunk of the report if the original report is
chunked when uploaded.
Besides, it also sets the channel label (stable/experimental) based on a
new property in the report,
run_info.browser_channel, introduced inweb-platform-tests/wpt#12679 .
Review
The integration, including the modified upload process for full raw reports, is tested by deploying to staging.wpt.fyi. The heuristics for adding labels are tested in unit tests.